feat: #217 - onboarding screenshot generation. #1529

monsieurtanuki · 2022-04-10T09:02:28Z

New files:

app_test.dart: Onboarding screenshot generation.
screenshot_driver.dart: Screenshot driver.

Impacted files:

analytics_helper.dart: added a special case for screenshot init.
main.dart: refactored for screenshots, and more specifically moved init code to be run before calling the app.
next_button.dart: added a Key for test/screenshots
Podfile.lock: wtf
pubspec.lock: impacted by pubspec.yaml
pubspec.yaml: added integration_test and flutter_driver to dev_dependencies.

What

Screenshot generation for onboarding.
First screenshot generation of the app: it works but there's some fine-tuning to do, regarding initial data and bad FutureBuilder management by the test framework.
More info available in app_test.dart (how to star the screenshots) and screenshot_driver.dart (how to make that work in iOS)

Screenshot

Part of

Automatic screenshot generation for Marketing & Tests #217

New files: * `app_test.dart`: Onboarding screenshots. * `screenshot_driver.dart`: Screenshot driver. Impacted files: * `analytics_helper.dart`: added a special case for screenshot init. * `main.dart`: refactored for screenshots, and more specifically moved init code to be run before calling the app. * `next_button.dart`: added a `Key` for test/screenshots * `Podfile.lock`: wtf * `pubspec.lock`: impacted by `pubspec.yaml` * `pubspec.yaml`: added integration_test and flutter_driver to dev_dependencies.

Impacted files: * `pubspec.lock`: impacted by `pubspec.yaml`. * `pubspec.yaml`: upgraded the version numbers.

teolemon · 2022-04-10T09:30:01Z

That's awesome @monsieurtanuki
So, if I want to run this from #1208 , I would just need to run the integration tests ?
Classicaly, I built a modified version of the app + specific APKs (in the case of Android), and run a gradle command to launch generation

monsieurtanuki · 2022-04-10T09:42:51Z

So, if I want to run this from #1208 , I would just need to run the integration tests ?

That's what you have to run:

flutter drive --driver=test_driver/screenshot_driver.dart --target=integration_test/app_test.dart

As is, that generates the screenshot files with hard-coded names in a new screenshots folder (same level as lib)
That means that there is definitely some customization to do, regarding:

the "root" folder for the screenshot files (so far, always the same)
the filenames themselves (so far, always the same filenames)
in order to make different names (language, country?, platform)
probably with ENV variables

teolemon · 2022-04-10T09:54:40Z

also device size / models (especially for iOS)

M123-dev

Heyyy thanks a lot @monsieurtanuki, automated screenshots ate definitely great. I actually have a stale integration tests branch it's a great opportunity to add further testing together with the screenshots. The majority looks good but please have a look at my comments.

M123-dev · 2022-04-10T09:57:31Z

packages/smooth_app/integration_test/app_test.dart

+Future<void> _initScreenshot(
+  final IntegrationTestWidgetsFlutterBinding binding,
+) async {
+  if ((!kIsWeb) && Platform.isAndroid) {


It can't be Web when it's android or am I missing something

You're right, but I explicitly say that I handle the kIsWeb case - which wouldn't be obvious without that additional test. Or it would be a comment: I prefer code to comment.

M123-dev · 2022-04-10T10:05:19Z

packages/smooth_app/lib/main.dart

+    if (!_screenshots) {
+      await _userPreferences.init(_productPreferences);
+    }
+    AnalyticsHelper.initMatomo(context, _screenshots);


You can just put it in the if statement above

No I can't: I do things in AnalyticsHelper.initMatomo(context, _screenshots);.

M123-dev · 2022-04-10T10:09:46Z

packages/smooth_app/lib/main.dart

+        if (!_screenshots) {
+          FlutterNativeSplash.remove();
+        }


Can you add a comment why we what it looks like never remove the splash screen

M123-dev · 2022-04-10T10:13:45Z

packages/smooth_app/integration_test/app_test.dart

+        await app.main(screenshots: true);
+        await tester.pumpAndSettle();
+
+        sleep(const Duration(seconds: 30));


I think pumpAndSettle already takes care of waiting

I'm not sure of anything anymore. Especially when there's async load: ok the pumpAndSettle will tell you that the display is refreshed, but not that the data was loaded async'ly in order to refresh the display.

M123-dev · 2022-04-10T10:16:59Z

packages/smooth_app/lib/main.dart

+    if (!_screenshots) {
+      await _userPreferences.init(_productPreferences);
+    }
+    AnalyticsHelper.initMatomo(context, _screenshots);


We don't need _screenshots as argument as it's a global variable

I don't think it is: it's private.

M123-dev · 2022-04-10T10:18:20Z

packages/smooth_app/lib/helpers/analytics_helper.dart

-  static void initMatomo(final BuildContext context) {
+  static void initMatomo(
+    final BuildContext context,
+    final bool screenshotMode,


We don't need this parameter as it's a global value

M123-dev · 2022-04-10T10:18:47Z

packages/smooth_app/lib/helpers/analytics_helper.dart

+    final bool screenshotMode,
+  ) {
+    if (screenshotMode) {
+      setCrashReports(false);


What about putting the same logic in initSentry

It's not that I particulary care about Matomo rather than Sentry; it's just that initMatomo froze the app in screenshot mode. And I'm sort of forced to call initMatomoso that the next matomo calls don't crash.
In screenshot mode I don't even call initSentry.

M123-dev · 2022-04-10T10:22:45Z

info • Avoid print calls in production code • packages/smooth_app/test_driver/screenshot_driver.dart:27:5 • avoid_print

Impacted files: * `main.dart`: added a comment. * `screenshot_driver.dart`: removed a "forbidden" `print` statement.

…openfoodfacts#217

codecov-commenter · 2022-04-10T18:10:05Z

Codecov Report

Merging #1529 (8fcb62a) into develop (342f8c0) will decrease coverage by 0.00%.
The diff coverage is 15.78%.

@@            Coverage Diff             @@
##           develop   #1529      +/-   ##
==========================================
- Coverage     8.90%   8.89%   -0.01%     
==========================================
  Files          161     161              
  Lines         6581    6596      +15     
==========================================
+ Hits           586     587       +1     
- Misses        5995    6009      +14

Impacted Files	Coverage Δ
...kages/smooth_app/lib/helpers/analytics_helper.dart	`0.00% <0.00%> (ø)`
packages/smooth_app/lib/main.dart	`17.89% <14.70%> (-1.62%)`	⬇️
...s/smooth_app/lib/pages/onboarding/next_button.dart	`4.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 342f8c0...8fcb62a. Read the comment docs.

M123-dev · 2022-04-11T12:35:13Z

@monsieurtanuki this whole analytics stuff looks a bit confusing to me, we call the initialization but return before the real call and first set the analytics to what is saved in the preferences then overwrite it when in screenshot mode. Instead of putting in the mock preferences, but let's not block it on this we can always improve that part later.

monsieurtanuki · 2022-04-11T12:45:30Z

Thank you @M123-dev!
If you feel confused about this PR it's because I explained very well ;)
The screenshots are a mess and a pain in the neck. No logic involved. As I said before there are fine-tuning and refactoring to do, the tricky part being to always check that we've still got screenshots generated.

monsieurtanuki requested a review from a team as a code owner April 10, 2022 09:02

monsieurtanuki added 2 commits April 10, 2022 11:06

feat: openfoodfacts#217 - merge conflict fix.

ea99b2f

Impacted files: * `pubspec.lock`: impacted by `pubspec.yaml`. * `pubspec.yaml`: upgraded the version numbers.

Merge branch 'develop' into feat/openfoodfacts#217

f27c45b

M123-dev reviewed Apr 10, 2022

View reviewed changes

teolemon assigned monsieurtanuki Apr 10, 2022

teolemon added the 📢 Screenshot generation label Apr 10, 2022

teolemon linked an issue Apr 10, 2022 that may be closed by this pull request

Automatic screenshot generation for Marketing & Tests #217

Closed

4 tasks

teolemon removed a link to an issue Apr 10, 2022

Automatic screenshot generation for Marketing & Tests #217

Closed

4 tasks

monsieurtanuki added 2 commits April 10, 2022 20:07

feat: openfoodfacts#217 - after code review.

7a46adb

Impacted files: * `main.dart`: added a comment. * `screenshot_driver.dart`: removed a "forbidden" `print` statement.

Merge remote-tracking branch 'origin/feat/openfoodfacts#217' into feat/…

8fcb62a

…openfoodfacts#217

monsieurtanuki requested a review from M123-dev April 10, 2022 19:09

teolemon added this to the V1.1 milestone Apr 11, 2022

M123-dev approved these changes Apr 11, 2022

View reviewed changes

monsieurtanuki merged commit e23117d into openfoodfacts:develop Apr 11, 2022

github-actions bot mentioned this pull request Apr 11, 2022

chore(develop): release 1.0.0 #962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #217 - onboarding screenshot generation. #1529

feat: #217 - onboarding screenshot generation. #1529

monsieurtanuki commented Apr 10, 2022

teolemon commented Apr 10, 2022

monsieurtanuki commented Apr 10, 2022

teolemon commented Apr 10, 2022

M123-dev left a comment

M123-dev Apr 10, 2022

monsieurtanuki Apr 10, 2022

M123-dev Apr 10, 2022

monsieurtanuki Apr 10, 2022

M123-dev Apr 10, 2022

M123-dev Apr 10, 2022

monsieurtanuki Apr 10, 2022

M123-dev Apr 10, 2022

monsieurtanuki Apr 10, 2022

M123-dev Apr 10, 2022

M123-dev Apr 10, 2022

monsieurtanuki Apr 10, 2022

M123-dev commented Apr 10, 2022

codecov-commenter commented Apr 10, 2022

M123-dev commented Apr 11, 2022

monsieurtanuki commented Apr 11, 2022

feat: #217 - onboarding screenshot generation. #1529

feat: #217 - onboarding screenshot generation. #1529

Conversation

monsieurtanuki commented Apr 10, 2022

What

Screenshot

Part of

teolemon commented Apr 10, 2022

monsieurtanuki commented Apr 10, 2022

teolemon commented Apr 10, 2022

M123-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M123-dev commented Apr 10, 2022

codecov-commenter commented Apr 10, 2022

Codecov Report

M123-dev commented Apr 11, 2022

monsieurtanuki commented Apr 11, 2022